Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add B904 - Use raise ... from err in except clauses #181

Merged
merged 5 commits into from
Sep 9, 2021
Merged

Add B904 - Use raise ... from err in except clauses #181

merged 5 commits into from
Sep 9, 2021

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Aug 22, 2021

Closes #180, by adding a new check B018 B904 to recommend exception chaining. For example,

try:
    assert False
except AssertionError:
    raise RuntimeError  # B904: Within an except clause, use `raise ... from err` ...

This turned out to be remarkably easy to add to the existing code 😁

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - Thanks!

My only questions are (And this is open to others):

  • Should this be optional?
  • Should this be only on >= 3.9 (Is that when this feature was introduced?)

Also, what triggers W503? I'm guessing some code you didn't add ...

  • Can we # nova: W503 them so we can look to clean them up?

@@ -2,7 +2,7 @@
# Keep in sync with setup.cfg which is used for source packages.

[flake8]
ignore = E203, E302, E501, E999
ignore = E203, E302, E501, E999, W503
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this? Just your personal preference? I usually follow it and move the operator.

Copy link
Member Author

@Zac-HD Zac-HD Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following black's recommendation - W503 is actually disabled by default, but enabled for bugbear by the W in select = B,C,E,F,W,B9.

I updated the config because I also added the first case of this pattern, in check_for_b018() here, and black insists on that placement of the operator.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use extend-ignore instead of ignore here to not override the defaults?

W503 for instance is off by default unless defaults are overridden by ignore configuration.

README.rst Show resolved Hide resolved
README.rst Outdated
Comment on lines 133 to 137
**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
or ``raise ... from None`` to distinguish them from errors in exception handling.
See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
for details.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to care to make this only care if we're >= 3.9?

Suggested change
**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
or ``raise ... from None`` to distinguish them from errors in exception handling.
See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
for details.
**B018**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
or ``raise ... from None`` to distinguish them from errors in exception handling (i.e. prefer exception chaining).
See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
for details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception chaining was added in Python 3.0! It's just not in widespread use yet, due to the syntatic incompatibility with Python 2 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O - I went to docs here: https://docs.python.org/3.9/tutorial/errors.html#exception-chaining

  • Set it to 3.8 and this section disappears ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I guess it's not been well documented... the reference docs still don't have a heading for it, though ctrl-f chain does find some mentions.

@cooperlees
Copy link
Collaborator

Will give it till the morning pacific - If no one says anything against this I'll merge.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 24, 2021

I've added a cheeky extra clause to allow raise some_lowercase_name, because both Hypothesis and Trio use this pattern when constructing an exception object with a specific traceback - and it seems unlikely to silence the warning on code which is of concern.

@hukkin
Copy link

hukkin commented Aug 25, 2021

Should this be optional?

I do think that the from e syntax should in theory always be used when raising within an exception handler. In practice however, I find myself too lazy to write out the extra characters, as the benefit is very little, especially if the re-raised exception will be caught later on and never bubbles up to a traceback. Sometimes the added characters make the line long enough that it needs to be wrapped in more than one line making the expression slightly less readable.

Just some reasoning why making this optional should perhaps be considered. Personally don't feel strongly either way.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 25, 2021

Fair points @hukkin - you can always disable it in global config though, and IMO if something is off by default it might as well not exist for most users 😕

@hukkin
Copy link

hukkin commented Aug 25, 2021

Yeah I agree that off-by-default warnings are not much use.

Thinking a bit more about this from a library developer perspective, there's two types of errors that will be raised:

  • Errors that are raised when a downstream developer (consumer of the library) makes incorrect use of the API --> we want the best traceback possible
  • Errors that are handled internally within the library and should never be seen by downstream developers --> we don't care about the traceback

What would be great is if this warning could only be applied to the first error type, but that isn't possible really. So I feel the question is which one is more important: having a warning for the first error type, or not needlessly annoying developers raising errors of the second error type.

Application developers also may find this annoying as they should generally always shut down gracefully and never let an error bubble all the way up.

Then there's the argument that all software has bugs, so any error may bubble up even though not intended to do so...

I think I'm starting to lean more towards "this is probably more annoying than useful".

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 25, 2021

Here's the diff in Hypothesis, which has plenty of both examples:

Patched from err from None
user-facing 2 6
internal 4 1

Personally I think that even the internal changes are net-positive, in that the code now expresses that these exceptions were raised because of rather than while handling the previous error, and that the user-facing improvements are enough to be worth some inconvenience anyway.

It's easy to turn off the check if you don't like it, but exception chaining is amazingly under-used simply because very few people know it exists and it wasn't viable for libraries until we stopped supporting Python 2 - even just saying "hey, this exists" is useful!

@cooperlees
Copy link
Collaborator

cooperlees commented Aug 25, 2021

I for one, am in the haven't used it bucket. I need to sit down and really understand when it's valuable and worth using etc. (If anyone has great tutorial they recommend shoot them my way or it might even be worth adding to the readme here)

I've enjoyed this discussion. But now I'm torn with default or optional. Any people passing by I'd love your thoughts before we merge this. I'll give it another few days. I'm leaning towards the default. Why:

  • This reminds me of B015 (pointless comparison) in that people loved it found unittests doing nothing and we received thanks (which is rare for an OSS project) for it!
  • I feel this check will open people up to making libraries now with more helpful exceptions etc.
  • As you've both stated, disabling is easy in config
    • Thanks to things like black most people have .flake8 configs now ...

@hukkin
Copy link

hukkin commented Aug 26, 2021

I think the best way to understand this is to try out the three different ways of exception chaining:

Disable chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two") from None
... 
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Implicit chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Explicit chaining

>>> try:
...     raise Exception("one")
... except Exception as e:
...     raise Exception("two") from e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Why I think the feature isn't that widely used (in addition to Python 2 compat) is that the default (implicit chaining) is pretty good: it doesn't hide information. I fear that rather than receiving thanks, we may receive complaints for an overly opinionated check with this one.

@cooperlees
Copy link
Collaborator

I think the best way to understand this is to try out the three different ways of exception chaining:

Disable chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two") from None
... 
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Implicit chaining

>>> try:
...     raise Exception("one")
... except Exception:
...     raise Exception("two")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Explicit chaining

>>> try:
...     raise Exception("one")
... except Exception as e:
...     raise Exception("two") from e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
Exception: one

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
Exception: two

Why I think the feature isn't that widely used (in addition to Python 2 compat) is that the default (implicit chaining) is pretty good: it doesn't hide information. I fear that rather than receiving thanks, we may receive complaints for an overly opinionated check with this one.

I kind of also like the default behavior and am leaning towards hukkin's points now. So we have people each way here. What do we want to do? I think there is noise here. We could make it optional for people who want it @Zac-HD ?

@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 4, 2021

I'd still prefer it on-by-default, but you're the maintainer - if you want it optional, I'll make it optional 🙂

@cooperlees
Copy link
Collaborator

I would love a second maintainer (all thought most are not active these days) thoughts if any swing by ....

This is useful when you want to explicitly modify the traceback, as done by Hypothesis and Trio
@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 7, 2021

@asottile @The-Compiler @graingert - do you think we should recommend raise ... from err by default, or only if specifically enabled?

@asottile
Copy link
Member

asottile commented Sep 7, 2021

some questions:

  • without from you still get exception chaining, but with from it changes the message slightly right?
  • how does this interact with from_traceback?
  • will individuals respond to the error incorrectly and slap from None on things? (many times the correct response is raise e => raise)

(I actually don't think I have the commit bit here, but stop by from time to time so feel free to discount my questions / opinions)

@The-Compiler
Copy link
Member

To get an idea of the impact, I ran this over both qutebrowser (~160 instances) and pytest (~300 instances). Both probably wouldn't profit too much from the new warning: qutebrowser because it's an application so it hardly matters; pytest due to how it's in charge of how it prints its own tracebacks anyways.

I guess flake8-bugbear users appreciate a more nitpicky linter compared to "plain" flake8 users, but on the other side that's often a reason people don't like pylint, because despite all configurability, at some point it drives people away if a linter produces more noise than signal by default.

I suppose it boils down to discoverability vs. a low-noise default configuration. For some projects I suspect this will be rather useless as others said, but of course bugbear doesn't know which ones those would be.

FWIW I've also rarely seen raise ... from ... in the wild, so even if useful, it's not exactly a best practice (yet?) I suppose.

I'm avoiding a concrete answer to the question because I really don't know. I'd end up disabling the check and not mind much, but others might feel differently.

@Zac-HD Zac-HD changed the title Add B018 - Use raise ... from err in except clauses Add B904 - Use raise ... from err in except clauses Sep 8, 2021
@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 8, 2021

OK; that's enough to convince me that disabled-by-default is sensible - and change made, it's now B904.

  • without from you still get exception chaining, but with from it changes the message slightly right?

Yep, you get exception changing regardless, from just improves the message (or from None hides earlier tracebacks).

  • how does this interact with from_traceback?

If you mean dis.Bytecode.from_traceback() (or BaseException.with_traceback()), it's handled the same way regardless of from. The docs aren't great, but introspection uses the __context__ attribute which isn't affected - from just sets the __cause__.

  • will individuals respond to the error incorrectly and slap from None on things? (many times the correct response is raise e => raise)

Quite possibly - though raise e wouldn't trigger this check, since I allow raising an all-lowercase name. This does suggest another new lint rule though...

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this will be safer from noise. If people come round wanting this on by default we can look at changing.

Will try update CHANGES etc. + release this week.

@cooperlees cooperlees merged commit b462a5e into PyCQA:master Sep 9, 2021
@Zac-HD Zac-HD deleted the B018-raise-from branch September 9, 2021 06:05
jkglasbrenner added a commit to usnistgov/dioptra that referenced this pull request Sep 23, 2021
Satisfy the new B904 rule (PyCQA/flake8-bugbear#181) from `flake8-bugbear`
by using explicit exception chaining whenever an exception is being transformed.
jkglasbrenner added a commit to usnistgov/dioptra that referenced this pull request Sep 28, 2021
Satisfy the new B904 rule (PyCQA/flake8-bugbear#181) from `flake8-bugbear`
by using explicit exception chaining whenever an exception is being transformed.
github-actions bot added a commit to usnistgov/dioptra that referenced this pull request Sep 28, 2021
Satisfy the new B904 rule (PyCQA/flake8-bugbear#181) from `flake8-bugbear`
by using explicit exception chaining whenever an exception is being transformed. 29ee504
charliermarsh added a commit to astral-sh/ruff that referenced this pull request Jul 14, 2023
## Summary

It looks like bugbear, [from the
start](PyCQA/flake8-bugbear#181 (comment)),
has had an exemption here to exempt `raise lower_case_var`. I looked at
Hypothesis and Trio, which are mentioned in that issue, and Hypothesis
has exactly one case of this, and Trio has none, so IMO it doesn't seem
worth special-casing.

Closes #5664.
evanrittenhouse pushed a commit to evanrittenhouse/ruff that referenced this pull request Jul 19, 2023
## Summary

It looks like bugbear, [from the
start](PyCQA/flake8-bugbear#181 (comment)),
has had an exemption here to exempt `raise lower_case_var`. I looked at
Hypothesis and Trio, which are mentioned in that issue, and Hypothesis
has exactly one case of this, and Trio has none, so IMO it doesn't seem
worth special-casing.

Closes astral-sh#5664.
konstin pushed a commit to astral-sh/ruff that referenced this pull request Jul 19, 2023
## Summary

It looks like bugbear, [from the
start](PyCQA/flake8-bugbear#181 (comment)),
has had an exemption here to exempt `raise lower_case_var`. I looked at
Hypothesis and Trio, which are mentioned in that issue, and Hypothesis
has exactly one case of this, and Trio has none, so IMO it doesn't seem
worth special-casing.

Closes #5664.
@yuwash
Copy link

yuwash commented Dec 12, 2023

Sorry I’m really not sure about. With the controversy already shown above, I think it should be off by default. It only “improves” the phrasing of the error message a bit although chaining itself happens by default. It seems rather hacky to try to change the wording by introducing such a linter rule rather than making the change to python itself. The referenced part of the docs notably doesn’t say one should always use from. It only says “useful when you are transforming exceptions” whereas I’m not completely sure what counts as “transforming”.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposed check: explicit raise in except or finally should use from ...
6 participants